First steps to move to SQLDelight #47

raphaelm · 2024-07-19T10:12:26Z

No description provided.

raphaelm · 2024-07-19T10:14:31Z

libpretixsync/build.gradle

+            packageName = "eu.pretix.libpretixsync.sqldelight"
+            srcDirs('src/main/sqldelight/sqlite', 'src/main/sqldelight/common', 'src/main/sqldelight/migrations')
+
+            version = 105


I'm worried we'll forget to keep this in sync between build.gradle and build-postgres.gradle, can we move it to its own gradle file and import it from there?

I'm wondering if this is needed at all, since the version it deducts from the migration is prioritized anyway? I'll check if we can just remove it. If not, I agree that it should go into a common file

raphaelm · 2024-07-19T11:56:48Z

libpretixsync/src/main/java/eu/pretix/libpretixsync/sync/OrderSyncAdapter.kt

+        for (i in 0 until checkins.length()) {
+            val ci = checkins.getJSONObject(i)
+            val listid = ci.getLong("list")
+            if (known.containsKey(listid)) {


I see that it was the same in the old code, but reading this… this is wrong, isn't it? How did this ever work? We would need to use the server ID of the checkin here, not the list ID?

@robbi5 what do you think?

raphaelm · 2024-09-11T15:10:12Z

libpretixsync/src/main/java/eu/pretix/libpretixsync/models/Question.kt

+    override fun getOptions(): List<QuestionOption>? = _options
+
+    override fun requiresAnswer(): Boolean = required
+


The implementation of getDependencyValues is missing! This breaks dependencies between questions.

Here's a good event for testing:
https://staging.pretix.eu/control/event/postest/questions/questions/

Fixed in bd295fb.

Should we make getDependency() and getDependencyValues() abstract in QuestionLike? Or are there many cases that rely on the defaults?

raphaelm · 2024-09-16T16:13:35Z

libpretixsync/src/main/java/eu/pretix/libpretixsync/sync/BlockedTicketSecretSyncAdapter.kt

+    }
+
+    override fun update(obj: BlockedTicketSecret, jsonobj: JSONObject) {
+        // TODO: Test new behaviour. Original version had no update case


There is a specialty about blocked secrets: We don't care about a BlockedTicketSecret with blocked=false. The only reason it exists on the API layer is to allow for differential sync during updates. But there is no need to store it. So instead of updating the blocked field to 0, we can and should just delete the local database entry.

Correction, the old code does change the blocked field to false, but it never inserts a new row with blocked=false, and the new code does the same, so maybe this is correct this way.

We also have the clean-up call (db.blockedTicketSecretQueries.deleteNotBlocked()) in download(). That should clean up any rows that have been updated to blocked=false.
The only thing I wonder is if we can simplify any of this to make it more maintainable (e.g. no special treatment in insert()/update() and just a clean-up in download() maybe?). The performance impact of inserting / updating a few rows too many shouldn't be too significant (?)

raphaelm · 2024-09-16T16:33:02Z

libpretixsync/src/main/java/eu/pretix/libpretixsync/sync/BaseDownloadSyncAdapter.kt

+                    if (autoPersist()) {
+                        insert(jsonobj)
+                        inserted += 1
+                    }


Since we do no batching any more, autoPersist() does not make much sense any more. In fact, with autoPersist returning false, as e.g. the case for BlockedTicketSecretSyncAdapter, no data is saved any more!

Agree. Now that the implementations of insert/update contain the DB queries, there is no need to distinguish on BaseDownloadSyncAdapter level. And with the current code it's very easy to make the mistake of defaulting to false like I did in the BlockedTicketSecretSyncAdapter

kkostov · 2024-11-12T09:56:36Z

libpretixsync/src/main/java/eu/pretix/libpretixsync/sqldelight/AndroidUtilDateAdapter.kt

+import java.util.Date
+import java.util.TimeZone
+
+class AndroidUtilDateAdapter : ColumnAdapter<Date, String> {


Can this adapter be used by JVM as well or is it really android specific?

It's specific to how requery saved Date values as strings to the DB on Android. While we get 2024-11-13T09:49:28Z there, it uses a different format on a plain JVM (2024-11-13 09:49:28.210).
In theory, you can use this adapter in other places. But you probably want the JavaUtilDateAdapter for backwards compatibility on the JVM.

We found a bug in there actually. You might want to check out a566d2d

It returns the result of apiCall, which is non-null.

Since we no longer inherit from classes like AbstractReceipt, the JSON utils must be added separately.

Removes the positionid of the add-on parent from the JSON and replaces it with just the addon_to ID. With SQLDelight, we can no longer directly resolve the relation with just the addon_to field, which means that we have to pass in either the ReceiptLine that addon_to points to or its positionid. Since the toJSON() is mostly used for logging purposes and the positionid is not strictly required, we can simplify the function here and add the equivalent of addon_to.positionid to the JSON afterwards in the caller where required.

Provide the same functionality as the getters on AbstractItem, but for the SQLDelight query result.

Question models were missing the getDependencyValues() override.

We were providing the wrong type of Instant.

Some query building code remains which should be migrated in a separate step.

SQLDelight returns the Long value directly on Postgres while it wraps it in a result type on SQLDelight.

…droid is too old for TRUE/FALSE (PRETIXPOS-1HH)

Since SimpleDateFormat is not thread safe, we might be running into race conditions here actually. Re-creating a new instance is expensive, so this is only a temporary fix while we validate a change to DateTimeFormatter.

Should guarantee thread safety without constantly re-creating formatters.

Reduces warnings when we convert them to models.

The change in cce5bf9 fixed booleans on older SQLite versions, but broke the queries for Postgres. This is a workaround that should make both databases happy. Since Postgres is extra pedantic in CASE epxressions, the complex order position queries had to be moved to the set of dialect-specific queries.

Updates with 1 instead of TRUE also don't work on Postgres.

raphaelm changed the title ~~Sqldelight~~ First steps to move to SQLDelight Jul 19, 2024

antweb force-pushed the sqldelight branch from 3e2bfd1 to 9454d65 Compare July 19, 2024 11:45

raphaelm commented Jul 19, 2024

View reviewed changes

antweb force-pushed the sqldelight branch 3 times, most recently from d7ede36 to 04b5936 Compare July 25, 2024 15:39

antweb force-pushed the sqldelight branch 3 times, most recently from d835bc1 to 58026f1 Compare July 31, 2024 15:27

raphaelm commented Sep 11, 2024

View reviewed changes

raphaelm commented Sep 16, 2024

View reviewed changes

antweb force-pushed the sqldelight branch from c341799 to b7907ea Compare October 18, 2024 14:28

antweb force-pushed the sqldelight branch 3 times, most recently from 41615cb to 8b87835 Compare November 1, 2024 17:41

robbi5 marked this pull request as ready for review November 7, 2024 15:28

robbi5 approved these changes Nov 7, 2024

View reviewed changes

kkostov reviewed Nov 12, 2024

View reviewed changes

antweb added 11 commits November 22, 2024 15:32

Set up SQLDelight

2c3d7cc

Set up SQLDelight database and initial queries

2df8dba

Remove nullability from downloadFile response

189238d

It returns the result of apiCall, which is non-null.

Create base classes for SQLDelight sync adapters

e78758f

Convert Item sync adapter

4fe3894

Convert Cashier sync adapter

1566802

Create extensions with JSON utilities

77cce26

Since we no longer inherit from classes like AbstractReceipt, the JSON utils must be added separately.

Create Item extensions

b3f2a7d

Provide the same functionality as the getters on AbstractItem, but for the SQLDelight query result.

Add additional tables

bcb2d4f

Move Settings.fiscalJSON extension to core-pos-sdk

cf3bc0b

antweb and others added 27 commits November 22, 2024 15:32

Fix question dependencies

f3b9cca

Question models were missing the getDependencyValues() override.

Fix java.time to Joda conversion

975715e

We were providing the wrong type of Instant.

Migrate remaining usage of CheckInList

7ad4ee6

Migrate remaining usage of Item and ItemCategory

0148e06

Expand wildcard imports in AsyncCheckProviderTest

34104f5

Migrate remaining usage of ReusableMedium

2d88158

Migrate majority of OrderPosition in AsyncCheckProvider

66d5e78

Some query building code remains which should be migrated in a separate step.

Migrate search query in AsyncCheckProvider

f97bb6a

Expand wildcard import in OrderSyncTest

a514446

Migrate remaining usage of ResourceSyncStatus

e038275

Fix typing issue between SQLite and Postgres

3e2287f

SQLDelight returns the Long value directly on Postgres while it wraps it in a result type on SQLDelight.

Fix now datetime override in tests

01c3fae

Migrate AsyncCheckProvider.status()

7abbd2d

Add missing listed_price to item variation

4937a4a

Migrate OrderCleanup

2e399fd

Migrate remaining usage of Order / OrderPosition

bb1cf20

Migrate remaining usage of QueuedCall

41150ea

Migrate remaining usage of QueuedCheckIn

51ea1e8

Remove dataStore arg from check providers

6fb7d8d

Add query to select BadgeLayoutItem by event slug

a7c0e29

Migrate remaining usage of Closing

b95c50e

Migrate remaining usage of QueuedOrder

52f3b3b

Use 0/1 for booleans in the queries, the shipped sqlite version on an…

9d1d180

…droid is too old for TRUE/FALSE (PRETIXPOS-1HH)

Remove shared SimpleDateFormat instances in date adapters

357a01d

Since SimpleDateFormat is not thread safe, we might be running into race conditions here actually. Re-creating a new instance is expensive, so this is only a temporary fix while we validate a change to DateTimeFormatter.

Use DateTimeFormatter in date adapters

5a444bc

Should guarantee thread safety without constantly re-creating formatters.

Add issue_giftcard to test Items

7701aaf

Reduces warnings when we convert them to models.

antweb force-pushed the sqldelight branch from 707279d to 3a08d8b Compare November 22, 2024 14:32

antweb added 2 commits November 22, 2024 18:03

Remove id parameter from QueuedOrder INSERT

434c9f1

Move query to lock QueuedOrder to compat queries

eb80c46

Updates with 1 instead of TRUE also don't work on Postgres.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First steps to move to SQLDelight #47

First steps to move to SQLDelight #47

raphaelm commented Jul 19, 2024

raphaelm Jul 19, 2024

antweb Sep 20, 2024 •

edited

Loading

raphaelm Jul 19, 2024

raphaelm Sep 11, 2024

antweb Sep 27, 2024

raphaelm Sep 16, 2024 •

edited

Loading

raphaelm Sep 16, 2024

antweb Sep 20, 2024

raphaelm Sep 16, 2024

antweb Sep 20, 2024

antweb Sep 20, 2024

kkostov Nov 12, 2024

antweb Nov 13, 2024

antweb Nov 22, 2024

		override fun getOptions(): List<QuestionOption>? = _options

		override fun requiresAnswer(): Boolean = required

First steps to move to SQLDelight #47

Are you sure you want to change the base?

First steps to move to SQLDelight #47

Conversation

raphaelm commented Jul 19, 2024

Choose a reason for hiding this comment

antweb Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelm Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antweb Sep 20, 2024 •

edited

Loading

raphaelm Sep 16, 2024 •

edited

Loading